Update to Gradle 8.5 (and OpenSearch 2.11.1 baseline) #28
Conversation
|
@noCharger and @jngz-es, would you be able to add your reviews to this PR as well. |
There was a problem hiding this comment.
I think this PR is a master class in many (maybe all) of the refactorings done from 2.8 to 2.11. Would it be worth a blog post or some documentation to help anyone who has been stuck (like I was) upgrading a plugin through all of these changes?
I'd like to see if @noCharger, @mkhludnev, and @jngz-es would like to take a look at this before approval.
| @@ -233,7 +233,7 @@ public Query doToQuery(LtrQueryContext context, FeatureSet featureSet, Map<Strin | |||
| Script script = new Script(this.script.getType(), this.script.getLang(), | |||
| this.script.getIdOrCode(), this.script.getOptions(), nparams); | |||
| ScoreScript.Factory factoryFactory = context.getQueryShardContext().compile(script, ScoreScript.CONTEXT); | |||
| ScoreScript.LeafFactory leafFactory = factoryFactory.newFactory(nparams, context.getQueryShardContext().lookup()); | |||
| ScoreScript.LeafFactory leafFactory = factoryFactory.newFactory(nparams, context.getQueryShardContext().lookup(), context.getQueryShardContext().searcher()); | |||
There was a problem hiding this comment.
this is more about the newFactory function, than this review; does it make sense that we're now asking for two parameters from the same context.getQueryShardContext()? Could this parameter list continue to grow?
There was a problem hiding this comment.
this is more about the newFactory function, than this review; does it make sense that we're now asking for two parameters from the same context.getQueryShardContext()?
The factory does not know about QueryShardContext, it only needs lookup and searcher that could be provided by the context.
| @@ -111,7 +110,7 @@ public StoredFeature(String name, List<String> params, String templateLanguage, | |||
| } | |||
|
|
|||
| public StoredFeature(String name, List<String> params, String templateLanguage, XContentBuilder template) { | |||
| this(name, params, templateLanguage, Strings.toString(Objects.requireNonNull(template)), false); | |||
| this(name, params, templateLanguage, Objects.requireNonNull(template).toString(), false); | |||
There was a problem hiding this comment.
ooh. this is much more standardized than the hackery I was working on previously. nice!
| @@ -134,7 +133,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws | |||
| } else { | |||
| builder.field(TEMPLATE.getPreferredName()); | |||
| // it's ok to use NamedXContentRegistry.EMPTY because we don't really parse we copy the structure... | |||
| XContentParser parser = XContentFactory.xContent(template).createParser(NamedXContentRegistry.EMPTY, | |||
| XContentParser parser = MediaTypeRegistry.xContent(template).xContent().createParser(NamedXContentRegistry.EMPTY, | |||
There was a problem hiding this comment.
this also seems redundant (based on method names) and nests too deeply. Is there another way to not call a method called xContent twice (looking for readability). otherwise, is this something we should think about updating in core?
There was a problem hiding this comment.
I think the core is still in the middle of changes related to XContent, I think this is the way API is structured at the moment (but please feel free to suggest the other way of I am missing something)
| @@ -219,14 +218,16 @@ public Query doToQuery(LtrQueryContext context, FeatureSet set, Map<String, Obje | |||
|
|
|||
| private XContentParser createParser(Object source, NamedXContentRegistry registry) throws IOException { | |||
| if (source instanceof String) { | |||
| return XContentFactory.xContent((String) source).createParser(registry, LoggingDeprecationHandler.INSTANCE, (String) source); | |||
| return MediaTypeRegistry.xContent((String) source).xContent() | |||
There was a problem hiding this comment.
this xContent().xContent() call is really bugging me, but I definitely don't have another suggestion.
There was a problem hiding this comment.
The whole MediaTypeRegistry.xContent is deprecated (with the goal to not "guess" the content type)
There was a problem hiding this comment.
OK. so this is evolving. Are there other issues open in core or explanation of these deprecated methods that can be linked here so we can plan upgrades better?
There was a problem hiding this comment.
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
Description
Update to Gradle 8.5 (and OpenSearch 2.11.1 baseline)
Issues Resolved
Part of opensearch-project/OpenSearch#10334
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.